-
Notifications
You must be signed in to change notification settings - Fork 193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ability to add child BONChainables to style text between tags. #144
Conversation
a5312f4
to
52fb458
Compare
@Imperiopolis cool, thanks for posting this! I'm looking forward to digging in to it. A question to start things off: why do you have the option to supply custom start, end and escape characters? Is it something you're using in your app? I ask because the syntax for .tagStyles(@{@"bold": boldChain, @"italic": italicChain}) Of course, this looks better in Swift: .tagStyles(["bold": boldChain, "italic": italicChain]) |
Also, taking a straight dictionary of .tagStyles(Appearance.defaultStyles + ["specialCase": specialChain]) (Can you tell I'm ready to be done with Obj-C?) |
I initially wrote this exactly as you described, but as I was tidying things up I realized that the system was flexible enough to allow any arbitrary tag and it seemed worth exposing. Some use cases I see for this:
For my particular uses, the former is not super useful but I intend to use the latter. I am utilizing this as a lightweight sort of HTML parser, such that my copy can essentially be shared between mobile and web with web using CSS styles and mobile using BonMot. There are other libraries with more fully flushed out ways to handle complex HTML like this (utilizing CSS), but they all seem to incur a pretty significant performance hit to build out the full DOMTree. Since the styling I need to support is quite simple, I built out this lightweight version in BonMot to accommodate. All that said, I think it could still be reasonably easy to accommodate your dictionary method by adding a function that takes the dictionary and converts it to the appropriate simple .tagStyles(Appearance.defaultStyles + [BONTagMake("specialCase", specialChain)]) Will need to think on it a bit. |
52fb458
to
0102251
Compare
I added in a .tagStyles(BONTagsFromDictionary(Appearance.defaultStyles + ["specialCase": specialChain])) Also considering adding a .tagStylesDictionary(Appearance.defaultStyles + ["specialCase": specialChain]) |
I like this. Maybe make the default one, which takes a |
That makes sense. I'll make that tweak :)
|
…mplexStyles to handle advanced cases.
1f56e1e
to
993f2a1
Compare
BONChain *italicChain = BONChain.new.fontNameAndSize(@"Baskerville-Italic", 15); | ||
|
||
BONChain *baseChain = BONChain.new.fontNameAndSize(@"Baskerville", 17) | ||
.tagStyles(@[ BONTagMake(@"bold", boldChain), BONTagMake(@"italic", italicChain) ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#req build warning: Incompatible pointer types passing 'NSArray *' to parameter of type 'NSDictionary<NSString *,id<BONTextable>> * _Nullable'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ this results in a crash if you scroll down to the bottom in the sample app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops, forgot to update this one when I switched the default to take a dictionary.
@Imperiopolis it would be nice if |
@@ -66,7 +64,12 @@ | |||
CD6DF05A1BF6B98500676E2D /* NSDictionary+BONEquality.m in Sources */ = {isa = PBXBuildFile; fileRef = CD6DF04F1BF6B53100676E2D /* NSDictionary+BONEquality.m */; }; | |||
CD78701E1CA5F8EC00B2714F /* EBGaramond12-Regular.otf in Resources */ = {isa = PBXBuildFile; fileRef = CD6DF03E1BF6AFAA00676E2D /* EBGaramond12-Regular.otf */; }; | |||
CDE658581C24ADD8009C7D09 /* BONUnderlineAndStrikethroughTestCase.m in Sources */ = {isa = PBXBuildFile; fileRef = CDE658571C24ADD8009C7D09 /* BONUnderlineAndStrikethroughTestCase.m */; }; | |||
EC2F199E1CEA324900D9F1FE /* BONTagDictionaryStylesTestCase.m in Sources */ = {isa = PBXBuildFile; fileRef = EC2F199C1CEA324300D9F1FE /* BONTagDictionaryStylesTestCase.m */; }; | |||
EC433DB31C88B7D9001B3ABE /* BONTagStylesTestCase.m in Sources */ = {isa = PBXBuildFile; fileRef = EC433DB11C88B65B001B3ABE /* BONTagStylesTestCase.m */; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#req rename the test case files to match what they're testing (they weren't updated when you changed the names of the tag style properties).
I see a few advantages to having an explicit close tag for all start tags:
That said, there's nothing stopping this method from being used as |
Shared parsing across platforms is a good case. I don't think nested tags is, though: you can easily do nested tags with How bad/slow would it make the code to support |
Yes, I suppose you can do nested tags with For example, it's pretty unclear what's happening here: Which one of these was this meant to be? What was the extraneous trailing Where as with explicit tags, you can quickly realize that "oh, forgot the opening h2" And that this pretty clearly produces:
There is no performance impact switching to |
@Imperiopolis OK, you've convinced me with the "makes editing harder" argument. Let's leave |
* | ||
* @return An array of ranges representing the escaped tags. | ||
*/ | ||
+ (BONNonnull BONGeneric(NSArray, NSValue *) *)escapedRangesInString:(NSString *BONCNonnull *BONCNonnull)string withTags:(BONNonnull BONGeneric(NSArray, BONTag *) *)tags stripEscapeCharacters:(BOOL)stripEscapeCharacters; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#req document the fact that *string
is mutated to contain the version of the string with unescaped tags stripped.
|
||
#pragma mark - Tag Matching | ||
|
||
+ (NSArray *)escapedRangesInString:(NSString **)string withTags:(NSArray *)tags stripEscapeCharacters:(BOOL)stripEscapeCharacters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#req please search the project for NSArray *
and replace with BONGeneric
@Imperiopolis is it intentional that you can have different escape characters for each tag in a scenario where you're using complex tags? What's the use case? |
} | ||
} | ||
|
||
+ (NSArray *)rangesInString:(NSString **)string betweenTags:(NSArray *)tags stripTags:(BOOL)stripTags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#? why would you ever not strip tags? You never pass NO
here, in the tests or the example app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, there really is no reason I can think of. Will remove this code path.
@Imperiopolis I may have missed something, but I don't think you test escaping the escape character anywhere. |
|
||
[escapedRanges addObject:[NSValue valueWithRange:range]]; | ||
|
||
searchRange = NSMakeRange(NSMaxRange(range), [theString length] - NSMaxRange(range)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#pp theString.length
for idempotent getters
@Imperiopolis OK, I’m done with my initial pass at the code review. Tiny tweaks may crop up, but in general it’s looking fantastic. Thanks so much for all the work you've put into this! |
Escaping the escape character isn't something this currently supports – what's the goal of this? To handle if for some reason you needed a |
I think the major reason to support escaping the escape character is because people would expect that to work. I tried putting |
Yes. I'm not really sure there is a great use case for this, but since these are complex options I figured it'd be nice to expose it if someone needed it. |
Are there tests for cases with different escapes for different tags? |
b8ea5ba
to
624ca58
Compare
There are now! |
624ca58
to
f2350f2
Compare
This adds the ability to parse and style substrings between arbitrary tags with assigned
BONChainable
objects. This potentially resolves #69.Excerpt from the readme:
Tag Styles
BonMot can style text between arbirtrary tags using a
<tag></tag>
format and\
as an escape character.Outputs:
BonMot can also style text between any arbitrary start and end strings using any escape string.
Note: Tag styles do not support nested or interleaved tags. The first tag matched will be applied, any additional tags between the start end end will be ignored.